CodeQL 6: refactor: catch specific exceptions across CTS/RAPS/shared#194
CodeQL 6: refactor: catch specific exceptions across CTS/RAPS/shared#194rlorenzo wants to merge 7 commits into
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (28)
📝 WalkthroughWalkthroughThe PR narrows many broad catch-all exception handlers to filtered catches for specific exception types and adds required using/import directives where needed. ChangesException Handling Specificity
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #194 +/- ##
==========================================
- Coverage 43.17% 43.16% -0.02%
==========================================
Files 893 893
Lines 51810 51817 +7
Branches 4841 4840 -1
==========================================
- Hits 22370 22365 -5
- Misses 28894 28907 +13
+ Partials 546 545 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
f559040 to
fd88995
Compare
d888563 to
5e071de
Compare
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
Refactors exception handling across CTS/ClinicalScheduler/RAPS/shared utilities to reduce CodeQL cs/catch-of-all-exceptions alerts by replacing broad catch (Exception)/catch { } blocks with more specific exception filters.
Changes:
- Narrowed many
catch (Exception)blocks usingwhen (ex is …)filters across controllers, middleware, and utility/services. - Tightened previously bare
catch { }blocks to explicit exception types (e.g., JSON parsing, email validation). - Added a top-level startup
CA1031suppression inProgram.csto keep a broad catch at the application boundary.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| web/ViteProxyHelpers.cs | Narrows proxy/fallback exception handling. |
| web/Services/EmailService.cs | Narrows exception handling in service availability check. |
| web/Program.cs | Narrows several startup/runtime catches; adds CA1031 suppression for top-level startup catch. |
| web/Classes/Utilities/IamApi.cs | Narrows catch in IAM response parsing. |
| web/Classes/Utilities/F5HttpRequest.cs | Narrows catch around HttpClient send/fallback logic. |
| web/Classes/Utilities/ActiveDirectoryService.cs | Narrows catches around AD group membership mutations. |
| web/Classes/UserHelper.cs | Narrows catches around user lookup/caching helpers. |
| web/Classes/SitemapMiddleware.cs | Narrows catch in sitemap generation middleware. |
| web/Classes/ClaimsTransformer.cs | Narrows catch during claims/roles assignment. |
| web/Areas/RAPS/Services/VMACSExport.cs | Narrows catches for VMACS export HTTP call and response parsing. |
| web/Areas/RAPS/Services/OuGroupService.cs | Narrows catch around AD group retrieval. |
| web/Areas/RAPS/Controllers/RoleMembersController.cs | Tightens VMACS JSON parse from bare catch to JsonException. |
| web/Areas/RAPS/Controllers/RolesController.cs | Narrows DB update catch in role creation/update path. |
| web/Areas/RAPS/Controllers/AdGroupsController.cs | Narrows catch when creating AD groups. |
| web/Areas/Computing/Services/BiorenderStudentLookup.cs | Tightens email validation exception handling. |
| web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs | Narrows catches before rethrowing to ApiExceptionFilter. |
| web/Areas/ClinicalScheduler/Controllers/InstructorScheduleController.cs | Narrows action-level catches for schedule mutations/queries. |
| web/Areas/ClinicalScheduler/Controllers/PermissionsController.cs | Narrows catches before rethrowing to ApiExceptionFilter. |
| web/Areas/ClinicalScheduler/Controllers/RotationsController.cs | Narrows catches before rethrowing to ApiExceptionFilter. |
| web/Areas/CTS/Controllers/BundleCompetencyController.cs | Narrows catch during DB mutation. |
| web/Areas/CTS/Controllers/BundleCompetencyGroupController.cs | Narrows catch during DB mutation. |
| web/Areas/CTS/Controllers/BundleController.cs | Narrows catch during DB mutation. |
| web/Areas/CTS/Controllers/CompetencyController.cs | Narrows catch during DB mutation. |
| web/Areas/CTS/Controllers/CourseController.cs | Narrows catch during DB mutation (multiple sites). |
| web/Areas/CTS/Controllers/DomainController.cs | Narrows catch during DB mutation. |
| web/Areas/CTS/Controllers/EpaController.cs | Narrows catch during DB mutation. |
| web/Areas/CTS/Controllers/LevelsController.cs | Narrows catch during DB mutation/transaction. |
| web/Areas/CTS/Controllers/RoleController.cs | Narrows catch during DB mutation. |
Comments suppressed due to low confidence (13)
web/ViteProxyHelpers.cs:376
- HandleProxyError’s fallback-to-static-files catch filter is too narrow/mismatched: filesystem and SendFileAsync paths can throw UnauthorizedAccessException (not an IOException), IOException/DirectoryNotFoundException, etc., but won’t throw DbUpdateException/SqlException. As written, some real fallback failures will now bubble out and skip the intended 502 response/logging. Catch the relevant IO/security exceptions here (and avoid catching OperationCanceledException so request aborts propagate cleanly).
try
{
// Determine the static file path
string? staticPath = null;
if (context.Request.Path.StartsWithSegments("/vue") || context.Request.Path.StartsWithSegments("/2/vue"))
{
staticPath = context.Request.Path.Value?.Replace("/2/vue", "/vue") ?? "/vue";
}
else
{
// Check if it's a Vue app route that should map to index.html
var pathValue = context.Request.Path.Value;
foreach (var appName in vueAppNames)
{
if (string.Equals(pathValue, $"/{appName}", StringComparison.OrdinalIgnoreCase) ||
pathValue?.StartsWith($"/{appName}/", StringComparison.OrdinalIgnoreCase) == true)
{
staticPath = $"/vue/src/{appName}/index.html";
break;
}
}
// If no app match, map as asset file
if (staticPath == null)
{
staticPath = "/vue/src" + context.Request.Path;
}
}
var physicalPath = Path.Join(context.RequestServices.GetRequiredService<IWebHostEnvironment>().WebRootPath,
staticPath.TrimStart('/').Replace('/', Path.DirectorySeparatorChar));
// Prevent directory traversal: ensure the resolved physical path is within WebRootPath
var webRoot = context.RequestServices.GetRequiredService<IWebHostEnvironment>().WebRootPath;
var resolvedPhysical = Path.GetFullPath(physicalPath);
var resolvedWebRoot = Path.GetFullPath(webRoot);
if (!resolvedPhysical.StartsWith(resolvedWebRoot, StringComparison.OrdinalIgnoreCase))
{
// Do not serve files outside the web root
logger.LogWarning("Attempt to access file outside web root: {Path}", resolvedPhysical);
}
else if (File.Exists(resolvedPhysical))
{
var contentType = GetContentType(Path.GetExtension(resolvedPhysical));
// Set content type if not already started
context.Response.ContentType = contentType ?? "application/octet-stream";
await context.Response.SendFileAsync(resolvedPhysical);
return;
}
}
catch (Exception fileEx) when (fileEx is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException or IOException)
{
var safePath = context.Request.Path.ToString().Replace("\r", "").Replace("\n", "");
logger.LogWarning(fileEx, "Failed to serve static file fallback for {Path}", safePath);
}
web/Program.cs:460
- The Vite dev-server proxy catch is filtering for EF/SQL exceptions, but SendAsync/CopyProxyResponse failures will be HttpRequestException/IOException/etc. This means common “Vite not running” errors may no longer fall back to static files. Also, catching OperationCanceledException here can mis-handle request aborts as “Vite unavailable”. Catch the relevant HTTP/network exceptions and let cancellations propagate.
try
{
// Use the registered HttpClient from dependency injection
var httpClientFactory = context.RequestServices.GetRequiredService<IHttpClientFactory>();
var httpClient = httpClientFactory.CreateClient("ViteProxy");
// Build the Vite server URL and try to proxy directly
var viteUrl = ViteProxyHelpers.BuildViteUrl(context.Request.Path, context.Request.QueryString, VueAppNames);
var requestMessage = ViteProxyHelpers.CreateProxyRequest(context, viteUrl);
using var response = await httpClient.SendAsync(requestMessage, HttpCompletionOption.ResponseHeadersRead, context.RequestAborted);
// Copy the response back to the client
await ViteProxyHelpers.CopyProxyResponse(context, response);
return; // Successfully proxied, don't continue to static files
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
var logger = context.RequestServices.GetRequiredService<ILogger<Program>>();
logger.LogDebug(ex, "Vite server not available, falling back to static files for {Path}",
Uri.EscapeDataString(context.Request.Path.Value ?? "unknown"));
// Fall through to static file serving
}
web/Program.cs:590
- File.Delete(awsCredentialsFilePath) can throw IOException/UnauthorizedAccessException/DirectoryNotFoundException, but the current filter only matches EF/SQL/InvalidOperation/OperationCanceled. If deletion fails, the exception may now escape and fail startup unexpectedly. Catch the filesystem exceptions that File.Delete actually throws (and avoid catching OperationCanceledException here).
try
{
File.Delete(awsCredentialsFilePath);
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
logger.Error(ex, $"COULD NOT DELETE THE AWS CREDENTIALS XML FILE (\"{awsCredentialsFilePath}\"). The file will need to be deleted manually.");
logger.Error(ex, $"COULD NOT DELETE THE AWS CREDENTIALS XML FILE (\"{awsCredentialsFilePath}\"). The file will need to be deleted manually.");
}
web/Program.cs:590
- This catch logs the same error message twice, which can create duplicate alerts/noise. Remove the duplicate logger.Error call (or vary the messages if two distinct log entries are intended).
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
logger.Error(ex, $"COULD NOT DELETE THE AWS CREDENTIALS XML FILE (\"{awsCredentialsFilePath}\"). The file will need to be deleted manually.");
logger.Error(ex, $"COULD NOT DELETE THE AWS CREDENTIALS XML FILE (\"{awsCredentialsFilePath}\"). The file will need to be deleted manually.");
}
web/Areas/RAPS/Services/VMACSExport.cs:144
- The export HTTP call can fail with HttpRequestException/IOException/TaskCanceledException (network/F5 issues), but the current filter only matches EF/SQL exceptions, so failures may now escape without being recorded into the messages list. Catch the relevant HTTP/network exceptions for _f5HttpRequest.Send and let OperationCanceledException propagate if cancellation is supported.
try
{
using HttpResponseMessage response = await _f5HttpRequest.Send(request);
RecordMessage(messages, "Response Status: " + response.StatusCode);
VmacsResponse vmacsResponse = await ParseResponse(response);
RecordMessage(messages, JsonSerializer.Serialize(vmacsResponse));
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
HttpHelper.Logger.Log(LogLevel.Warn, ex);
RecordMessage(messages, "Error: " + ex.Message + " " + ex.StackTrace);
if (ex.InnerException != null)
{
RecordMessage(messages, "Error: " + ex.InnerException.Message + " " + ex.InnerException.StackTrace);
}
}
web/Areas/RAPS/Services/VMACSExport.cs:178
- VMACS response parsing errors will typically be JsonException/NotSupportedException from JsonSerializer.Deserialize; the current catch filter won’t match those, so invalid VMACS JSON can now throw instead of returning a structured error response. Catch the JSON parsing exceptions here (and avoid catching OperationCanceledException unless you rethrow it immediately).
private static async Task<VmacsResponse> ParseResponse(HttpResponseMessage response)
{
VmacsResponse? vmacsResponse;
string responseBody = await response.Content.ReadAsStringAsync();
try
{
vmacsResponse = JsonSerializer.Deserialize<VmacsResponse>(responseBody, new JsonSerializerOptions { PropertyNameCaseInsensitive = true });
if (vmacsResponse == null)
{
throw new InvalidOperationException("Failed to deserialize VMACS response.");
}
vmacsResponse.Success = response.IsSuccessStatusCode;
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
vmacsResponse = new()
{
Success = false,
ErrorMessage = "Invalid response from VMACS: " + responseBody
};
}
web/Areas/RAPS/Services/OuGroupService.cs:53
- ActiveDirectoryService.GetGroups() failures are likely directory/LDAP exceptions (e.g., PrincipalOperationException/LdapException), not EF/SQL exceptions. With the current filter, those errors will now bypass this handler and can crash the request. Update the filter to the AD/LDAP exception types actually thrown (and don’t catch OperationCanceledException unless rethrowing).
try
{
ActiveDirectoryService.GetGroups();
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
Logger logger = LogManager.GetCurrentClassLogger();
logger.Error(ex);
}
web/Classes/ClaimsTransformer.cs:111
- The catch filter excludes RecordNotFoundException, but the catch body explicitly checks for RecordNotFoundException to decide whether to log. As written, RecordNotFoundException will now bypass this handler and can break claims transformation/auth. Include RecordNotFoundException in the filter (and avoid catching OperationCanceledException here so request aborts don’t get logged/handled as 500s).
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
bool isProd = false;
if (HttpHelper.Environment != null)
{
isProd = HttpHelper.Environment.IsProduction();
}
// if were on the production system or the error is something other that RecordNotFoundException then log the error
// basically we don't want to be notified of RecordNotFound exception's except on production since users can be missing until the next db refresh
if (isProd || !(ex is RecordNotFoundException))
{
HttpHelper.Logger.Log(LogLevel.Error, ex, ex.Message);
}
}
web/Classes/UserHelper.cs:306
- These UserHelper catches now include OperationCanceledException and return null, which can convert request cancellation into “user not found” behavior. Also, the filter is very narrow for code that touches caching/DI/EF, so unexpected but recoverable exceptions may now escape. Prefer letting OperationCanceledException propagate, and ensure the filter matches the actual exception types you intend to handle for these helper methods.
try
{
AAUDContext? aaudContext = _aaudContext
?? (AAUDContext?)HttpHelper.HttpContext?.RequestServices.GetService(typeof(AAUDContext));
AaudUser? currentUser = GetByLoginId(aaudContext, HttpHelper.HttpContext?.User?.Identity?.Name);
return currentUser;
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
HttpHelper.Logger.Error(ex);
return null;
}
web/Areas/ClinicalScheduler/Controllers/RotationsController.cs:90
- Several controller-level catch filters include OperationCanceledException and then rethrow into ApiExceptionFilter, which will log the cancellation as an error and return a 500. Request aborts should generally propagate without being treated as server errors. Remove OperationCanceledException from these filters (or add a dedicated catch (OperationCanceledException) { throw; } before the filtered catch) throughout this controller.
_logger.LogDebug("Retrieved {Count} rotations, filtered to {FilteredCount} based on permissions",
rotations.Count, filteredRotations.Count);
return Ok(filteredRotations);
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
// Store context for ApiExceptionFilter to use in logging
SetExceptionContext(new Dictionary<string, object>
{
["ServiceId"] = serviceId,
["Operation"] = "RetrieveRotations"
});
throw; // Let ApiExceptionFilter handle the response
}
web/Areas/ClinicalScheduler/Controllers/PermissionsController.cs:163
- Several controller-level catch filters include OperationCanceledException and then rethrow into ApiExceptionFilter, which will log the cancellation as an error and return a 500. Request aborts should generally propagate without being treated as server errors. Remove OperationCanceledException from these filters (or add a dedicated catch (OperationCanceledException) { throw; } before the filtered catch) throughout this controller.
LogSanitizer.SanitizeId(user.MothraId), serviceId, canEdit);
return Ok(new { canEdit });
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
// Store context for ApiExceptionFilter to use in logging
SetExceptionContext("ServiceId", serviceId);
throw; // Let ApiExceptionFilter handle the response
}
}
web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs:583
- Several controller-level catch filters include OperationCanceledException and then rethrow into ApiExceptionFilter, which will log the cancellation as an error and return a 500. Request aborts should generally propagate without being treated as server errors. Remove OperationCanceledException from these filters (or add a dedicated catch (OperationCanceledException) { throw; } before the filtered catch) throughout this controller.
// All other cases (Admin, Manage, EditClnSchedules, rotation view, or service-specific permissions) see all clinicians
return clinicians;
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
_logger.LogError(ex, "Error filtering clinicians by permissions. Returning unfiltered list.");
return clinicians; // Return unfiltered list on error to avoid breaking functionality
}
web/Services/EmailService.cs:228
- IsServiceAvailableAsync doesn’t perform EF/SQL operations, so this exception filter is unlikely to ever match real failures and it introduces unnecessary coupling to EF Core/SqlClient. Either remove the try/catch entirely (since IsMailpitAvailableAsync already returns false on failure) or catch the specific networking exceptions that can occur during availability checks.
public async Task<bool> IsServiceAvailableAsync()
{
try
{
// If using Mailpit in development, check if it's running
if (_hostEnvironment.IsDevelopment() && _emailSettings.UseMailpit)
{
return await IsMailpitAvailableAsync();
}
// For production, assume the SMTP server is available
return true;
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
_logger.LogError(ex, "Error checking email service availability");
return false;
}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs (1)
579-583:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed on permission-filter exceptions.
Returning
clinicianshere exposes unfiltered data when permission checks fail. Return an empty list (or rethrow) instead of fail-open behavior.Suggested fix
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) { _logger.LogError(ex, "Error filtering clinicians by permissions. Returning unfiltered list."); - return clinicians; // Return unfiltered list on error to avoid breaking functionality + return new List<ClinicianSummary>(); // Fail closed on permission-evaluation errors }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs` around lines 579 - 583, The catch block that currently catches DbUpdateException/SqlException/InvalidOperationException/OperationCanceledException and calls _logger.LogError(...) must not fail open by returning the unfiltered variable clinicians; change the behavior to return an empty collection (e.g., Enumerable.Empty<Clinician>() or new List<Clinician>()) or rethrow the exception instead of returning clinicians to avoid exposing unfiltered data—update the catch clause in the CliniciansController method containing the catch (Exception ex) when (...) block and keep the _logger.LogError call but replace return clinicians with a safe empty-list return (or throw) as appropriate for your error-handling policy.web/Classes/ClaimsTransformer.cs (1)
95-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFilter breaks existing
RecordNotFoundExceptionhandling path.Line 95 narrows the catch so
RecordNotFoundExceptionis no longer reliably handled, but Lines 104–107 still assume it is. This can now bubble out of role-assignment flow unexpectedly. Add an explicitcatch (RecordNotFoundException)path (or restore intended behavior) before the other specific catches.As per coding guidelines, "
**/*.cs: Prefer specific exception types ... Flag broadExceptioncatches inside service/business logic."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Classes/ClaimsTransformer.cs` around lines 95 - 107, The current catch in ClaimsTransformer narrows Exception to DbUpdateException/SqlException/InvalidOperationException/OperationCanceledException which prevents the existing RecordNotFoundException handling from running; add an explicit catch (RecordNotFoundException) block (before the broader catches) in the method that contains this try/catch so RecordNotFoundException is handled locally as intended (or restore the original broader catch ordering), then keep the existing production-check/logging logic in the other catch blocks (the block referencing HttpHelper.Environment and the isProd check) to ensure RecordNotFoundException no longer bubbles out of the role-assignment flow.web/Program.cs (2)
454-460:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVite proxy fallback catch is too narrow; it won’t run on common HttpClient proxy failures.
In
web/Program.cs(around theHttpClient.SendAsynccatch), the filter only catchesDbUpdateException/SqlException/InvalidOperationException/OperationCanceledException, so typical network failures from a down/unreachable Vite server (e.g.,HttpRequestException, oftenIOException-wrapped) won’t trigger the “fall back to static files” path.Proposed fix
-catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) +catch (Exception ex) when (ex is HttpRequestException or TaskCanceledException or OperationCanceledException or IOException or InvalidOperationException) { var logger = context.RequestServices.GetRequiredService<ILogger<Program>>(); logger.LogDebug(ex, "Vite server not available, falling back to static files for {Path}", Uri.EscapeDataString(context.Request.Path.Value ?? "unknown")); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Program.cs` around lines 454 - 460, The catch block guarding the HttpClient.SendAsync fallback is too narrow and misses common network errors (e.g., HttpRequestException/IOException), so update the catch around the HttpClient.SendAsync call (the existing catch handling and logging using logger from context.RequestServices.GetRequiredService<ILogger<Program>> and the message "Vite server not available, falling back to static files for {Path}") to also catch HttpRequestException and IOException (and/or related network exceptions like SocketException) — or replace the filtered exception clause with a broader network-aware catch — then log the exception the same way and fall through to static file serving as before.
586-590:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
File.Deletecatch filter inweb/Program.cs.
File.Delete(awsCredentialsFilePath)is caught only for DB/SQL/InvalidOperation/OperationCanceled exceptions, so filesystem failures (IO/permissions/path/security/etc.) can escape the handler. Catch the relevant filesystem exception types instead.Proposed fix
-catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) +catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or SecurityException or NotSupportedException or PathTooLongException) { logger.Error(ex, $"COULD NOT DELETE THE AWS CREDENTIALS XML FILE (\"{awsCredentialsFilePath}\"). The file will need to be deleted manually."); logger.Error(ex, $"COULD NOT DELETE THE AWS CREDENTIALS XML FILE (\"{awsCredentialsFilePath}\"). The file will need to be deleted manually."); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Program.cs` around lines 586 - 590, The catch filter currently only checks for DB/SQL/InvalidOperation/OperationCanceled exceptions but the File.Delete(awsCredentialsFilePath) call can throw filesystem-related exceptions; update the when-clause on the catch for the File.Delete block to match filesystem exception types (e.g., IOException, UnauthorizedAccessException, PathTooLongException, NotSupportedException and SecurityException) so those failures are caught and logged by the existing logger.Error calls; locate the catch surrounding File.Delete in Program.cs (the catch with variable ex) and replace the type checks in the when clause accordingly (also remove the duplicate logger.Error if still present).web/Services/EmailService.cs (1)
224-228:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix email availability error handling: outer catch is irrelevant, inner probe swallows all exceptions.
IsServiceAvailableAsynccatchesDbUpdateException/SqlException, but when Mailpit is used the real probe isIsMailpitAvailableAsync, which has a barecatch { return false; }, so network/transport failures are already hidden before the outer filter can help.- Tighten
IsMailpitAvailableAsyncto catch specific exceptions (and log) instead of swallowing everything; then remove or narrow the outercatch (Exception ex) when (...)since it won’t cover the failures that matter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Services/EmailService.cs` around lines 224 - 228, IsServiceAvailableAsync's outer filtered catch is ineffective because IsMailpitAvailableAsync currently swallows all exceptions; update IsMailpitAvailableAsync to only catch specific exception types (e.g., HttpRequestException, TaskCanceledException/OperationCanceledException, SocketException or other transport/network exceptions you expect) and log the exception with _logger.LogError/LogWarning before returning false, and then remove or narrow the outer catch in IsServiceAvailableAsync (the catch (Exception ex) when (...) block) so it no longer tries to handle broad/irrelevant exceptions—this ensures network/transport failures from IsMailpitAvailableAsync surface properly and are logged.web/ViteProxyHelpers.cs (2)
372-376:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStatic-file fallback catch misses important file-system/security exceptions.
web/ViteProxyHelpers.cscurrently catchesExceptiononly forDbUpdateException | SqlException | InvalidOperationException | OperationCanceledException | IOException, so failures likeUnauthorizedAccessException,SecurityException, orNotSupportedExceptioncan bypass this handler and avoid the intended fallback response.Proposed fix
-catch (Exception fileEx) when (fileEx is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException or IOException) +catch (Exception fileEx) when (fileEx is IOException or UnauthorizedAccessException or SecurityException or NotSupportedException or PathTooLongException or InvalidOperationException) { var safePath = context.Request.Path.ToString().Replace("\r", "").Replace("\n", ""); logger.LogWarning(fileEx, "Failed to serve static file fallback for {Path}", safePath); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/ViteProxyHelpers.cs` around lines 372 - 376, The catch filter in ViteProxyHelpers (the catch(Exception fileEx) when (...) block) excludes critical filesystem/security exceptions so the static-file fallback can be skipped; update the exception filter to also include UnauthorizedAccessException, System.Security.SecurityException, NotSupportedException (and optionally PathTooLongException) so those exceptions are handled the same way, keeping the existing safePath sanitization and logger.LogWarning(fileEx, "Failed to serve static file fallback for {Path}", safePath) call; ensure you reference the same local variable fileEx and preserve the surrounding logic in the method where the catch block appears.
289-296:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix exception filters in
ViteProxyHelpersto match the header/file operations
- Header copy catch (around line 289) includes
DbUpdateException/SqlExceptionand omits common header-setting failures (e.g., argument/format), so those can bypass the log; drop DB/SQL types and include header-relevant exceptions.- Static-file fallback catch (around line 372) includes DB/SQL types and only filters
IOException/InvalidOperationException; drop DB/SQL types and include filesystem/access exceptions thatSendFileAsync/path resolution can throw.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/ViteProxyHelpers.cs` around lines 289 - 296, Update the exception filters in ViteProxyHelpers: in the header copy catch (the catch block handling headerEx used when setting response headers) remove DbUpdateException and SqlException and instead catch header-relevant errors such as ArgumentException and FormatException plus the existing InvalidOperationException, OperationCanceledException and IOException so header-setting failures are always logged; similarly update the static-file fallback catch (the catch around SendFileAsync/path resolution) to remove DbUpdateException and SqlException and include filesystem/access exceptions like FileNotFoundException, DirectoryNotFoundException, PathTooLongException, UnauthorizedAccessException, ArgumentException and FormatException in addition to IOException/InvalidOperationException so file/path errors are properly handled and logged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/Areas/Computing/Services/BiorenderStudentLookup.cs`:
- Line 98: In BiorenderStudentLookup.cs replace the filtered catch "catch
(Exception ex) when (ex is FormatException or ArgumentException)" with two
explicit catches—first "catch (FormatException ex)" and then "catch
(ArgumentException ex)"—inside the same method in class BiorenderStudentLookup,
keeping the existing error handling logic for both; if both handlers share
identical code, factor shared behavior into a small private helper method to
avoid duplication and ensure the original logging/response behavior remains
unchanged.
In `@web/Areas/CTS/Controllers/CompetencyController.cs`:
- Around line 180-183: The catch block in CompetencyController's delete handler
currently returns a misleading message "Could not remove domain..." when
catching
DbUpdateException/SqlException/InvalidOperationException/OperationCanceledException;
update the return BadRequest(...) call in that catch block to reference the
correct entity (e.g., "Could not remove competency. It may be linked to other
objects.") or use the appropriate localized/message constant so clients and logs
correctly state "competency" instead of "domain".
In `@web/Areas/CTS/Controllers/EpaController.cs`:
- Around line 109-112: The catch block in EpaController.cs that catches
DbUpdateException/SqlException/InvalidOperationException/OperationCanceledException
should not return raw ex.Message to clients; instead log the full exception
server-side (e.g., using the controller's ILogger or existing logging helper)
and return a fixed, non-sensitive BadRequest or Problem response (e.g., "An
error occurred processing your request."). Update the catch in the action method
to call logger.LogError(ex, "...") with contextual information and replace
return BadRequest(ex.Message) with a generic error response message.
In `@web/Areas/RAPS/Controllers/AdGroupsController.cs`:
- Around line 117-120: In the AdGroupsController catch block (the catch
(Exception ex) when (...) that currently returns ValidationProblem(ex.Message)),
stop returning raw exception text to clients: log the full exception via the
controller's logger (e.g., _logger.LogError(ex, "Error creating/updating ad
group")) and replace ValidationProblem(ex.Message) with a generic client-safe
response such as ValidationProblem("An error occurred while processing the
request.") or a ValidationProblemDetails instance containing a non-sensitive
message; preserve the original exception only in server logs for diagnostics.
In `@web/Areas/RAPS/Controllers/RolesController.cs`:
- Around line 226-228: In RolesController's catch block (the catch (Exception
ex) when (...) handler) remove appending ex.InnerException?.Message from the
Problem(...) response so the API returns a generic error like "There was a
problem updating the database."; instead log the full exception server-side
(e.g., _logger.LogError(ex, "Error updating roles") or similar) and return only
the generic Problem(...) call using the existing ex variable and Problem(...)
method to avoid leaking internal details.
In `@web/Areas/RAPS/Services/OuGroupService.cs`:
- Line 49: In OuGroupService replace the filtered catch "catch (Exception ex)
when (ex is DbUpdateException or SqlException or InvalidOperationException or
OperationCanceledException)" with explicit typed catch blocks: add separate
catches for DbUpdateException, SqlException, InvalidOperationException, and
OperationCanceledException in the method where the current catch appears (inside
the OuGroupService class), handling/logging each type as currently done for ex,
and avoid a broad catch(Exception); if a fallback is required, keep it minimal
(log and rethrow) but prefer the specific handlers.
In `@web/Areas/RAPS/Services/VMACSExport.cs`:
- Line 171: The catch filter in ParseResponse around the
JsonSerializer.Deserialize call currently only handles DbUpdateException,
SqlException, InvalidOperationException, and OperationCanceledException; update
that filter to also include JsonException (and optionally NotSupportedException)
so malformed VMACS JSON will be caught and the method can return the fallback
VmacsResponse; locate the JsonSerializer.Deserialize usage and the catch with
the filter in VMACSExport.ParseResponse and add the additional exception types
to the "ex is ..." list.
- Line 136: The catch block around the call to _f5HttpRequest.Send(request) in
VMACSExport.cs currently filters exceptions but omits HttpRequestException;
update the exception filter on the catch (currently "catch (Exception ex) when
(ex is DbUpdateException or SqlException or InvalidOperationException or
OperationCanceledException)") to also include HttpRequestException so
network/DNS/TLS failures are handled by this block (keep
OperationCanceledException for timeouts/cancellation and leave the other
exception types unchanged).
In `@web/Classes/SitemapMiddleware.cs`:
- Around line 90-93: The current catch in SitemapMiddleware that only catches
DbUpdateException/SqlException/InvalidOperationException/OperationCanceledException
prevents other sitemap-generation failures from falling through to
_next(context); change the handler to catch all exceptions at this middleware
boundary (e.g., catch(Exception ex)) so any runtime/reflection/type-loading
failures also await _next(context) and preserve fallback behavior; update the
catch block around the sitemap generation code (where the existing catch is
declared) and optionally add minimal logging inside the broad catch before
calling await _next(context).
In `@web/Classes/UserHelper.cs`:
- Line 275: Replace the filtered broad catch "catch (Exception ex) when (ex is
DbUpdateException or SqlException or InvalidOperationException or
OperationCanceledException)" used in the helper methods with explicit typed
catch blocks: add separate catches for DbUpdateException, SqlException,
InvalidOperationException, and OperationCanceledException (preserving the
exception variable name if used) and handle/log/rethrow each appropriately as
the original code intended; do this for every occurrence of that filtered catch
in UserHelper.cs (the three helper method catch sites) so each exception type is
caught explicitly rather than using a filtered Exception catch.
In `@web/Classes/Utilities/ActiveDirectoryService.cs`:
- Line 224: The catch filters in AddUserToGroup and RemoveUserFromGroup are
using DB/SQL exception types which are irrelevant to the
System.DirectoryServices.AccountManagement/LDAP operations; update the exception
filter to catch AD/LDAP-specific exceptions (e.g., PrincipalOperationException,
DirectoryServicesCOMException, LdapException and general
InvalidOperationException/OperationCanceledException if appropriate) and remove
DbUpdateException and SqlException so AD errors are correctly caught and
logged/handled by those methods.
In `@web/Classes/Utilities/F5HttpRequest.cs`:
- Around line 27-30: The retry catch currently filters
DbUpdateException/SqlException/InvalidOperationException which are irrelevant
for HttpClient; change the filter in the F5HttpRequest retry block so transport
failures trigger HandleConnectionFail by catching HttpRequestException and
keeping OperationCanceledException (to preserve timeout/cancellation behavior)
instead of the DB/SQL exception types — i.e., update the catch expression that
calls HandleConnectionFail(request, attemptNumber) to check for
HttpRequestException or OperationCanceledException.
In `@web/Classes/Utilities/IamApi.cs`:
- Around line 403-406: The exception filter in ParseResponse<T> currently
catches DbUpdateException, SqlException, InvalidOperationException, and
OperationCanceledException but misses JsonException and NotSupportedException so
JSON deserialize failures escape; update the catch in ParseResponse<T> to
include JsonException and NotSupportedException (or replace the filter with a
broader Exception catch for deserialize errors) and ensure the handler sets
r.ErrorMessage = "Invalid response: " + ex.Message + "." so
malformed/unsupported IAM payloads are converted into the error result.
In `@web/Program.cs`:
- Around line 90-93: The catch block that wraps
builder.Configuration.AddSystemsManager(...) is too narrow and lets non-matching
AWS/network exceptions escape; update the catch on the try surrounding
builder.Configuration.AddSystemsManager to either remove the restrictive
exception filter (catch Exception ex) or explicitly include
Amazon.Runtime.AmazonServiceException, Amazon.Runtime.AmazonClientException,
System.Net.Http.HttpRequestException and
TaskCanceledException/OperationCanceledException in the filter, then call
logger.Fatal(ex, "Failed to get secrets from AWS") as before and handle/rethrow
as appropriate; locate the catch block that currently references
DbUpdateException/SqlException and replace the filter with the recommended
exception types or a plain catch to ensure AWS/config/network errors are logged
and handled.
---
Outside diff comments:
In `@web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs`:
- Around line 579-583: The catch block that currently catches
DbUpdateException/SqlException/InvalidOperationException/OperationCanceledException
and calls _logger.LogError(...) must not fail open by returning the unfiltered
variable clinicians; change the behavior to return an empty collection (e.g.,
Enumerable.Empty<Clinician>() or new List<Clinician>()) or rethrow the exception
instead of returning clinicians to avoid exposing unfiltered data—update the
catch clause in the CliniciansController method containing the catch (Exception
ex) when (...) block and keep the _logger.LogError call but replace return
clinicians with a safe empty-list return (or throw) as appropriate for your
error-handling policy.
In `@web/Classes/ClaimsTransformer.cs`:
- Around line 95-107: The current catch in ClaimsTransformer narrows Exception
to
DbUpdateException/SqlException/InvalidOperationException/OperationCanceledException
which prevents the existing RecordNotFoundException handling from running; add
an explicit catch (RecordNotFoundException) block (before the broader catches)
in the method that contains this try/catch so RecordNotFoundException is handled
locally as intended (or restore the original broader catch ordering), then keep
the existing production-check/logging logic in the other catch blocks (the block
referencing HttpHelper.Environment and the isProd check) to ensure
RecordNotFoundException no longer bubbles out of the role-assignment flow.
In `@web/Program.cs`:
- Around line 454-460: The catch block guarding the HttpClient.SendAsync
fallback is too narrow and misses common network errors (e.g.,
HttpRequestException/IOException), so update the catch around the
HttpClient.SendAsync call (the existing catch handling and logging using logger
from context.RequestServices.GetRequiredService<ILogger<Program>> and the
message "Vite server not available, falling back to static files for {Path}") to
also catch HttpRequestException and IOException (and/or related network
exceptions like SocketException) — or replace the filtered exception clause with
a broader network-aware catch — then log the exception the same way and fall
through to static file serving as before.
- Around line 586-590: The catch filter currently only checks for
DB/SQL/InvalidOperation/OperationCanceled exceptions but the
File.Delete(awsCredentialsFilePath) call can throw filesystem-related
exceptions; update the when-clause on the catch for the File.Delete block to
match filesystem exception types (e.g., IOException,
UnauthorizedAccessException, PathTooLongException, NotSupportedException and
SecurityException) so those failures are caught and logged by the existing
logger.Error calls; locate the catch surrounding File.Delete in Program.cs (the
catch with variable ex) and replace the type checks in the when clause
accordingly (also remove the duplicate logger.Error if still present).
In `@web/Services/EmailService.cs`:
- Around line 224-228: IsServiceAvailableAsync's outer filtered catch is
ineffective because IsMailpitAvailableAsync currently swallows all exceptions;
update IsMailpitAvailableAsync to only catch specific exception types (e.g.,
HttpRequestException, TaskCanceledException/OperationCanceledException,
SocketException or other transport/network exceptions you expect) and log the
exception with _logger.LogError/LogWarning before returning false, and then
remove or narrow the outer catch in IsServiceAvailableAsync (the catch
(Exception ex) when (...) block) so it no longer tries to handle
broad/irrelevant exceptions—this ensures network/transport failures from
IsMailpitAvailableAsync surface properly and are logged.
In `@web/ViteProxyHelpers.cs`:
- Around line 372-376: The catch filter in ViteProxyHelpers (the catch(Exception
fileEx) when (...) block) excludes critical filesystem/security exceptions so
the static-file fallback can be skipped; update the exception filter to also
include UnauthorizedAccessException, System.Security.SecurityException,
NotSupportedException (and optionally PathTooLongException) so those exceptions
are handled the same way, keeping the existing safePath sanitization and
logger.LogWarning(fileEx, "Failed to serve static file fallback for {Path}",
safePath) call; ensure you reference the same local variable fileEx and preserve
the surrounding logic in the method where the catch block appears.
- Around line 289-296: Update the exception filters in ViteProxyHelpers: in the
header copy catch (the catch block handling headerEx used when setting response
headers) remove DbUpdateException and SqlException and instead catch
header-relevant errors such as ArgumentException and FormatException plus the
existing InvalidOperationException, OperationCanceledException and IOException
so header-setting failures are always logged; similarly update the static-file
fallback catch (the catch around SendFileAsync/path resolution) to remove
DbUpdateException and SqlException and include filesystem/access exceptions like
FileNotFoundException, DirectoryNotFoundException, PathTooLongException,
UnauthorizedAccessException, ArgumentException and FormatException in addition
to IOException/InvalidOperationException so file/path errors are properly
handled and logged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fac47195-2b26-4625-8406-e8b749f064a0
📒 Files selected for processing (28)
web/Areas/CTS/Controllers/BundleCompetencyController.csweb/Areas/CTS/Controllers/BundleCompetencyGroupController.csweb/Areas/CTS/Controllers/BundleController.csweb/Areas/CTS/Controllers/CompetencyController.csweb/Areas/CTS/Controllers/CourseController.csweb/Areas/CTS/Controllers/DomainController.csweb/Areas/CTS/Controllers/EpaController.csweb/Areas/CTS/Controllers/LevelsController.csweb/Areas/CTS/Controllers/RoleController.csweb/Areas/ClinicalScheduler/Controllers/CliniciansController.csweb/Areas/ClinicalScheduler/Controllers/InstructorScheduleController.csweb/Areas/ClinicalScheduler/Controllers/PermissionsController.csweb/Areas/ClinicalScheduler/Controllers/RotationsController.csweb/Areas/Computing/Services/BiorenderStudentLookup.csweb/Areas/RAPS/Controllers/AdGroupsController.csweb/Areas/RAPS/Controllers/RoleMembersController.csweb/Areas/RAPS/Controllers/RolesController.csweb/Areas/RAPS/Services/OuGroupService.csweb/Areas/RAPS/Services/VMACSExport.csweb/Classes/ClaimsTransformer.csweb/Classes/SitemapMiddleware.csweb/Classes/UserHelper.csweb/Classes/Utilities/ActiveDirectoryService.csweb/Classes/Utilities/F5HttpRequest.csweb/Classes/Utilities/IamApi.csweb/Program.csweb/Services/EmailService.csweb/ViteProxyHelpers.cs
5e071de to
d285401
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (3)
web/Program.cs:439
- The Vite dev-server proxy try/catch is filtering for database exceptions, so the common failure modes from HttpClient.SendAsync (e.g., HttpRequestException / TaskCanceledException) will bypass this catch and prevent the intended static-file fallback.
// Build the Vite server URL and try to proxy directly
var viteUrl = ViteProxyHelpers.BuildViteUrl(context.Request.Path, context.Request.QueryString, VueAppNames);
var requestMessage = ViteProxyHelpers.CreateProxyRequest(context, viteUrl);
using var response = await httpClient.SendAsync(requestMessage, HttpCompletionOption.ResponseHeadersRead, context.RequestAborted);
// Copy the response back to the client
await ViteProxyHelpers.CopyProxyResponse(context, response);
return; // Successfully proxied, don't continue to static files
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
var logger = context.RequestServices.GetRequiredService<ILogger<Program>>();
logger.LogDebug(ex, "Vite server not available, falling back to static files for {Path}",
Uri.EscapeDataString(context.Request.Path.Value ?? "unknown"));
// Fall through to static file serving
}
web/Program.cs:572
- File.Delete can throw IO/permission/path exceptions; the current exception filter only matches DB-related exceptions, so a delete failure will now escape and can break startup. Catch filesystem-related exceptions here, and avoid logging the same message twice.
try
{
File.Delete(awsCredentialsFilePath);
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
logger.Error(ex, $"COULD NOT DELETE THE AWS CREDENTIALS XML FILE (\"{awsCredentialsFilePath}\"). The file will need to be deleted manually.");
logger.Error(ex, $"COULD NOT DELETE THE AWS CREDENTIALS XML FILE (\"{awsCredentialsFilePath}\"). The file will need to be deleted manually.");
}
web/Services/EmailService.cs:228
- IsServiceAvailableAsync only checks environment flags and (optionally) calls IsMailpitAvailableAsync; the current catch filter for DbUpdateException/SqlException is unrelated and will not handle the actual connectivity errors (which are already handled inside IsMailpitAvailableAsync). This also makes the method harder to reason about and requires unnecessary imports.
try
{
// If using Mailpit in development, check if it's running
if (_hostEnvironment.IsDevelopment() && _emailSettings.UseMailpit)
{
return await IsMailpitAvailableAsync();
}
// For production, assume the SMTP server is available
return true;
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
_logger.LogError(ex, "Error checking email service availability");
return false;
}
CodeQL cs/catch-of-all-exceptions: same when-filter pattern as #193, applied across the remaining areas (~46 sites in 28 files): - CTS controllers (9 files) - ClinicalScheduler controllers (4 files) - RAPS controllers and services (5 files) - Computing service, shared web/Classes, web/Classes/Utilities (7 files) - web/Services/EmailService.cs, web/ViteProxyHelpers.cs, web/Program.cs Filter is restricted to DbUpdateException, SqlException, InvalidOperationException, OperationCanceledException (plus IOException for ViteProxyHelpers and FormatException/ArgumentException for the MailAddress email validator); anything outside that set now propagates. Two intentional broad catches kept with #pragma warning disable CA1031: - web/Program.cs top-level startup must catch any exception to log fatal before rethrowing. Bare `catch { }` in RoleMembersController (VMACS JSON parse) and BiorenderStudentLookup (MailAddress validation) tightened to the specific exceptions those calls actually throw.
…ndant qualifiers The codeql/6 batch script converted bare 'catch (Exception)' blocks to 'catch (Exception) when (true /* placeholder */)' to satisfy CodeQL. ReSharper rightly flagged that as CS7095 (filter is a constant). Replaced 14 of those with the standard when-filter pattern 'catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)' used in the rest of the PR. Also dropped Microsoft.EntityFrameworkCore. and Microsoft.Data.SqlClient. qualifiers in catch filters across 26 files; added the corresponding 'using' directives where missing.
Extends the catch-of-all cleanup to a probe that was added to EmailService after the original PR was written. A TCP connect fails with SocketException; other exception types now surface instead of being swallowed. The HangfireHealthCheck storage catch and EmailService.GetRequestContext are intentionally left broad: the former must not crash the health response on any storage error (it documents this), the latter is a defensive log-context builder that must never throw.
6991a87 to
7bd1864
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (3)
web/Classes/UserHelper.cs:264
- This catch filter includes
OperationCanceledException, which will swallow request-abort cancellation (returningnullinstead). Cancellation should propagate so upstream middleware can stop work and avoid misleading auth/permission behavior.
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
HttpHelper.Logger.Error(ex);
return null;
}
web/Classes/UserHelper.cs:291
- This catch filter includes
OperationCanceledException, which will turn request cancellation into anulluser. Prefer letting cancellation propagate (so the request aborts cleanly) and only handle DB/SQL/invalid-operation failures here.
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
HttpHelper.Logger.Error(ex);
return null;
}
web/Classes/UserHelper.cs:323
- This catch filter includes
OperationCanceledException, which will swallow request-abort cancellation (returningnull). Cancellation should propagate so the request can terminate promptly and consistently.
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
HttpHelper.Logger.Error(ex);
return null;
}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/Areas/Computing/Services/BiorenderStudentLookup.cs (1)
24-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSemaphore permits leak on invalid emails; this can hang the request.
throttler.WaitAsync()is called before validation (Line 26), but invalid emails skip task creation and never callthrottler.Release(). After enough invalid inputs, the loop blocks indefinitely. Acquire the semaphore only for valid emails (or release on the invalid branch).Suggested fix
foreach (var email in emails) { - await throttler.WaitAsync(); - var emailTrimmed = email.Trim(); if (!emailTrimmed.Contains('@')) { emailTrimmed += "`@ucdavis.edu`"; } - if (IsValidEmail(emailTrimmed)) - { - resultList.Add( - Task.Run(async () => - { - try - { - return await GetSingleStudent(emailTrimmed); - } - finally - { - throttler.Release(); - } - }) - ); - } + if (!IsValidEmail(emailTrimmed)) + { + continue; + } + + await throttler.WaitAsync(); + resultList.Add( + Task.Run(async () => + { + try + { + return await GetSingleStudent(emailTrimmed); + } + finally + { + throttler.Release(); + } + }) + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Areas/Computing/Services/BiorenderStudentLookup.cs` around lines 24 - 49, The loop currently calls throttler.WaitAsync() before validating emails, causing permits to leak when IsValidEmail(emailTrimmed) is false and no Release() is called; move the semaphore acquisition so it only occurs for valid emails (i.e., perform emailTrimmed/IsValidEmail(emailTrimmed) checks first), then call throttler.WaitAsync() immediately before creating the Task.Run that invokes GetSingleStudent(emailTrimmed) and ensure throttler.Release() remains in the Task's finally block (or alternatively, if you prefer keeping WaitAsync() early, add an explicit throttler.Release() on the invalid-email branch before continuing); update references around throttler.WaitAsync, IsValidEmail, GetSingleStudent, and resultList accordingly.web/ViteProxyHelpers.cs (1)
348-370:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep malformed request paths inside the static-file fallback.
This block now catches only IO/auth/invalid-operation failures, but it also normalizes a request-derived path with
Path.GetFullPath. Malformed or oversized paths can throwArgumentException,NotSupportedException, orPathTooLongException, which now escape the fallback and turn the intended 502 path into a 500. Catch the path-normalization exceptions here or validate the path before buildingphysicalPath.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/ViteProxyHelpers.cs` around lines 348 - 370, The path normalization (Path.GetFullPath) can throw ArgumentException, NotSupportedException, or PathTooLongException for malformed/oversized request paths, but the current catch only handles IO/Unauthorized/InvalidOperation; expand the exception filter on the catch (currently "when (fileEx is IOException or UnauthorizedAccessException or InvalidOperationException)") to also include ArgumentException, NotSupportedException, and PathTooLongException so those errors are handled by the static-file fallback (or alternatively validate/normalize the incoming staticPath before constructing physicalPath); refer to the physicalPath, resolvedPhysical, and the catch block surrounding Path.GetFullPath to locate the change.web/Classes/ClaimsTransformer.cs (1)
95-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLet request/database cancellation propagate through
web/Classes/ClaimsTransformer.csandweb/Classes/UserHelper.cs.
ClaimsTransformer.TransformAsync()andUserHelper.GetByLoginId(),GetCurrentUser(), andGetTrueCurrentUser()all includeOperationCanceledExceptionin filters that log and continue/returnnull. That turns aborted EF/request work into partial identities or missing users;web/Areas/ClinicalScheduler/Controllers/PermissionsController.cs:41-53already mapsGetCurrentUser() == nullto401 Unauthorized. RemoveOperationCanceledExceptionfrom all of these filters so cancellations do not masquerade as auth failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Classes/ClaimsTransformer.cs` around lines 95 - 109, In ClaimsTransformer.TransformAsync and in UserHelper.GetByLoginId, GetCurrentUser, and GetTrueCurrentUser remove OperationCanceledException from the exception filters so cancellations are not swallowed; specifically, update the catch expressions (e.g., the "catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)" in ClaimsTransformer) to exclude OperationCanceledException, and ensure cancellation exceptions are allowed to propagate (either by not catching them or by rethrowing them immediately) rather than logging and returning null.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs`:
- Around line 579-582: The permission helper methods currently only catch
DbUpdateException/SqlException/InvalidOperationException so non-DB failures from
GetCurrentUser(), GetRequiredService<RAPSContext>(), or HasPermission(...)
surface as 500s and break the intended fail-open/fail-closed contract; update
the helpers to catch all exceptions (catch Exception) and return the original
fallback values (e.g., return clinicians or return false) so
permission-resolution errors preserve the existing behavior, or alternatively
move the broad try/catch to the controller action boundary
(GetClinicians/GetClinicianSchedule) to preserve the contract — apply the same
change to both occurrences around the blocks referencing GetCurrentUser(),
GetRequiredService<RAPSContext>(), and HasPermission(...).
In `@web/Classes/Utilities/ActiveDirectoryService.cs`:
- Around line 222-225: Replace the filtered catch of a broad Exception with
explicit typed catches in the service/business logic: remove constructs like
"catch (Exception ex) when (ex is PrincipalException or
System.DirectoryServices.DirectoryServicesCOMException)" and add separate catch
blocks "catch (PrincipalException ex) { HttpHelper.Logger.Error(ex); }" and
"catch (System.DirectoryServices.DirectoryServicesCOMException ex) {
HttpHelper.Logger.Error(ex); }" (apply the same change in
ActiveDirectoryService.cs, F5HttpRequest.cs, IamApi.cs, and EmailService.cs),
ensure the appropriate namespaces are imported (e.g., System.DirectoryServices,
System.DirectoryServices.AccountManagement) and do not leave a broad
catch(Exception) in these non-boundary layers so other unexpected exceptions can
propagate.
In `@web/Program.cs`:
- Around line 473-475: The log is currently writing request-derived values
(e.g., context.Request.Path/queries/derived target URLs) using
Uri.EscapeDataString/HtmlEncode/newline-stripping which still allows
client-controlled data into logs; update the call sites in Program.cs
(viteLogger.LogDebug) and ViteProxyHelpers.cs to sanitize any request-originated
strings with LogSanitizer (e.g., LogSanitizer.SanitizeString or SanitizeId as
appropriate) before logging, and avoid logging raw query strings or full derived
target URLs—either omit them or log a sanitized/placeholder value instead.
Ensure you call the sanitizer on the specific variables passed into
LogDebug/LogInformation and do not log context.Request.QueryString or generated
target URL directly.
---
Outside diff comments:
In `@web/Areas/Computing/Services/BiorenderStudentLookup.cs`:
- Around line 24-49: The loop currently calls throttler.WaitAsync() before
validating emails, causing permits to leak when IsValidEmail(emailTrimmed) is
false and no Release() is called; move the semaphore acquisition so it only
occurs for valid emails (i.e., perform emailTrimmed/IsValidEmail(emailTrimmed)
checks first), then call throttler.WaitAsync() immediately before creating the
Task.Run that invokes GetSingleStudent(emailTrimmed) and ensure
throttler.Release() remains in the Task's finally block (or alternatively, if
you prefer keeping WaitAsync() early, add an explicit throttler.Release() on the
invalid-email branch before continuing); update references around
throttler.WaitAsync, IsValidEmail, GetSingleStudent, and resultList accordingly.
In `@web/Classes/ClaimsTransformer.cs`:
- Around line 95-109: In ClaimsTransformer.TransformAsync and in
UserHelper.GetByLoginId, GetCurrentUser, and GetTrueCurrentUser remove
OperationCanceledException from the exception filters so cancellations are not
swallowed; specifically, update the catch expressions (e.g., the "catch
(Exception ex) when (ex is DbUpdateException or SqlException or
InvalidOperationException or OperationCanceledException)" in ClaimsTransformer)
to exclude OperationCanceledException, and ensure cancellation exceptions are
allowed to propagate (either by not catching them or by rethrowing them
immediately) rather than logging and returning null.
In `@web/ViteProxyHelpers.cs`:
- Around line 348-370: The path normalization (Path.GetFullPath) can throw
ArgumentException, NotSupportedException, or PathTooLongException for
malformed/oversized request paths, but the current catch only handles
IO/Unauthorized/InvalidOperation; expand the exception filter on the catch
(currently "when (fileEx is IOException or UnauthorizedAccessException or
InvalidOperationException)") to also include ArgumentException,
NotSupportedException, and PathTooLongException so those errors are handled by
the static-file fallback (or alternatively validate/normalize the incoming
staticPath before constructing physicalPath); refer to the physicalPath,
resolvedPhysical, and the catch block surrounding Path.GetFullPath to locate the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 39ea5b09-3da0-452d-bc0f-c78e3aaf6749
📒 Files selected for processing (28)
web/Areas/CTS/Controllers/BundleCompetencyController.csweb/Areas/CTS/Controllers/BundleCompetencyGroupController.csweb/Areas/CTS/Controllers/BundleController.csweb/Areas/CTS/Controllers/CompetencyController.csweb/Areas/CTS/Controllers/CourseController.csweb/Areas/CTS/Controllers/DomainController.csweb/Areas/CTS/Controllers/EpaController.csweb/Areas/CTS/Controllers/LevelsController.csweb/Areas/CTS/Controllers/RoleController.csweb/Areas/ClinicalScheduler/Controllers/CliniciansController.csweb/Areas/ClinicalScheduler/Controllers/InstructorScheduleController.csweb/Areas/ClinicalScheduler/Controllers/PermissionsController.csweb/Areas/ClinicalScheduler/Controllers/RotationsController.csweb/Areas/Computing/Services/BiorenderStudentLookup.csweb/Areas/RAPS/Controllers/AdGroupsController.csweb/Areas/RAPS/Controllers/RoleMembersController.csweb/Areas/RAPS/Controllers/RolesController.csweb/Areas/RAPS/Services/OuGroupService.csweb/Areas/RAPS/Services/VMACSExport.csweb/Classes/ClaimsTransformer.csweb/Classes/SitemapMiddleware.csweb/Classes/UserHelper.csweb/Classes/Utilities/ActiveDirectoryService.csweb/Classes/Utilities/F5HttpRequest.csweb/Classes/Utilities/IamApi.csweb/Program.csweb/Services/EmailService.csweb/ViteProxyHelpers.cs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/Areas/Computing/Services/BiorenderStudentLookup.cs (1)
24-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSemaphore permits leak on invalid emails; this can hang the request.
throttler.WaitAsync()is called before validation (Line 26), but invalid emails skip task creation and never callthrottler.Release(). After enough invalid inputs, the loop blocks indefinitely. Acquire the semaphore only for valid emails (or release on the invalid branch).Suggested fix
foreach (var email in emails) { - await throttler.WaitAsync(); - var emailTrimmed = email.Trim(); if (!emailTrimmed.Contains('@')) { emailTrimmed += "`@ucdavis.edu`"; } - if (IsValidEmail(emailTrimmed)) - { - resultList.Add( - Task.Run(async () => - { - try - { - return await GetSingleStudent(emailTrimmed); - } - finally - { - throttler.Release(); - } - }) - ); - } + if (!IsValidEmail(emailTrimmed)) + { + continue; + } + + await throttler.WaitAsync(); + resultList.Add( + Task.Run(async () => + { + try + { + return await GetSingleStudent(emailTrimmed); + } + finally + { + throttler.Release(); + } + }) + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Areas/Computing/Services/BiorenderStudentLookup.cs` around lines 24 - 49, The loop currently calls throttler.WaitAsync() before validating emails, causing permits to leak when IsValidEmail(emailTrimmed) is false and no Release() is called; move the semaphore acquisition so it only occurs for valid emails (i.e., perform emailTrimmed/IsValidEmail(emailTrimmed) checks first), then call throttler.WaitAsync() immediately before creating the Task.Run that invokes GetSingleStudent(emailTrimmed) and ensure throttler.Release() remains in the Task's finally block (or alternatively, if you prefer keeping WaitAsync() early, add an explicit throttler.Release() on the invalid-email branch before continuing); update references around throttler.WaitAsync, IsValidEmail, GetSingleStudent, and resultList accordingly.web/ViteProxyHelpers.cs (1)
348-370:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep malformed request paths inside the static-file fallback.
This block now catches only IO/auth/invalid-operation failures, but it also normalizes a request-derived path with
Path.GetFullPath. Malformed or oversized paths can throwArgumentException,NotSupportedException, orPathTooLongException, which now escape the fallback and turn the intended 502 path into a 500. Catch the path-normalization exceptions here or validate the path before buildingphysicalPath.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/ViteProxyHelpers.cs` around lines 348 - 370, The path normalization (Path.GetFullPath) can throw ArgumentException, NotSupportedException, or PathTooLongException for malformed/oversized request paths, but the current catch only handles IO/Unauthorized/InvalidOperation; expand the exception filter on the catch (currently "when (fileEx is IOException or UnauthorizedAccessException or InvalidOperationException)") to also include ArgumentException, NotSupportedException, and PathTooLongException so those errors are handled by the static-file fallback (or alternatively validate/normalize the incoming staticPath before constructing physicalPath); refer to the physicalPath, resolvedPhysical, and the catch block surrounding Path.GetFullPath to locate the change.web/Classes/ClaimsTransformer.cs (1)
95-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLet request/database cancellation propagate through
web/Classes/ClaimsTransformer.csandweb/Classes/UserHelper.cs.
ClaimsTransformer.TransformAsync()andUserHelper.GetByLoginId(),GetCurrentUser(), andGetTrueCurrentUser()all includeOperationCanceledExceptionin filters that log and continue/returnnull. That turns aborted EF/request work into partial identities or missing users;web/Areas/ClinicalScheduler/Controllers/PermissionsController.cs:41-53already mapsGetCurrentUser() == nullto401 Unauthorized. RemoveOperationCanceledExceptionfrom all of these filters so cancellations do not masquerade as auth failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Classes/ClaimsTransformer.cs` around lines 95 - 109, In ClaimsTransformer.TransformAsync and in UserHelper.GetByLoginId, GetCurrentUser, and GetTrueCurrentUser remove OperationCanceledException from the exception filters so cancellations are not swallowed; specifically, update the catch expressions (e.g., the "catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)" in ClaimsTransformer) to exclude OperationCanceledException, and ensure cancellation exceptions are allowed to propagate (either by not catching them or by rethrowing them immediately) rather than logging and returning null.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs`:
- Around line 579-582: The permission helper methods currently only catch
DbUpdateException/SqlException/InvalidOperationException so non-DB failures from
GetCurrentUser(), GetRequiredService<RAPSContext>(), or HasPermission(...)
surface as 500s and break the intended fail-open/fail-closed contract; update
the helpers to catch all exceptions (catch Exception) and return the original
fallback values (e.g., return clinicians or return false) so
permission-resolution errors preserve the existing behavior, or alternatively
move the broad try/catch to the controller action boundary
(GetClinicians/GetClinicianSchedule) to preserve the contract — apply the same
change to both occurrences around the blocks referencing GetCurrentUser(),
GetRequiredService<RAPSContext>(), and HasPermission(...).
In `@web/Classes/Utilities/ActiveDirectoryService.cs`:
- Around line 222-225: Replace the filtered catch of a broad Exception with
explicit typed catches in the service/business logic: remove constructs like
"catch (Exception ex) when (ex is PrincipalException or
System.DirectoryServices.DirectoryServicesCOMException)" and add separate catch
blocks "catch (PrincipalException ex) { HttpHelper.Logger.Error(ex); }" and
"catch (System.DirectoryServices.DirectoryServicesCOMException ex) {
HttpHelper.Logger.Error(ex); }" (apply the same change in
ActiveDirectoryService.cs, F5HttpRequest.cs, IamApi.cs, and EmailService.cs),
ensure the appropriate namespaces are imported (e.g., System.DirectoryServices,
System.DirectoryServices.AccountManagement) and do not leave a broad
catch(Exception) in these non-boundary layers so other unexpected exceptions can
propagate.
In `@web/Program.cs`:
- Around line 473-475: The log is currently writing request-derived values
(e.g., context.Request.Path/queries/derived target URLs) using
Uri.EscapeDataString/HtmlEncode/newline-stripping which still allows
client-controlled data into logs; update the call sites in Program.cs
(viteLogger.LogDebug) and ViteProxyHelpers.cs to sanitize any request-originated
strings with LogSanitizer (e.g., LogSanitizer.SanitizeString or SanitizeId as
appropriate) before logging, and avoid logging raw query strings or full derived
target URLs—either omit them or log a sanitized/placeholder value instead.
Ensure you call the sanitizer on the specific variables passed into
LogDebug/LogInformation and do not log context.Request.QueryString or generated
target URL directly.
---
Outside diff comments:
In `@web/Areas/Computing/Services/BiorenderStudentLookup.cs`:
- Around line 24-49: The loop currently calls throttler.WaitAsync() before
validating emails, causing permits to leak when IsValidEmail(emailTrimmed) is
false and no Release() is called; move the semaphore acquisition so it only
occurs for valid emails (i.e., perform emailTrimmed/IsValidEmail(emailTrimmed)
checks first), then call throttler.WaitAsync() immediately before creating the
Task.Run that invokes GetSingleStudent(emailTrimmed) and ensure
throttler.Release() remains in the Task's finally block (or alternatively, if
you prefer keeping WaitAsync() early, add an explicit throttler.Release() on the
invalid-email branch before continuing); update references around
throttler.WaitAsync, IsValidEmail, GetSingleStudent, and resultList accordingly.
In `@web/Classes/ClaimsTransformer.cs`:
- Around line 95-109: In ClaimsTransformer.TransformAsync and in
UserHelper.GetByLoginId, GetCurrentUser, and GetTrueCurrentUser remove
OperationCanceledException from the exception filters so cancellations are not
swallowed; specifically, update the catch expressions (e.g., the "catch
(Exception ex) when (ex is DbUpdateException or SqlException or
InvalidOperationException or OperationCanceledException)" in ClaimsTransformer)
to exclude OperationCanceledException, and ensure cancellation exceptions are
allowed to propagate (either by not catching them or by rethrowing them
immediately) rather than logging and returning null.
In `@web/ViteProxyHelpers.cs`:
- Around line 348-370: The path normalization (Path.GetFullPath) can throw
ArgumentException, NotSupportedException, or PathTooLongException for
malformed/oversized request paths, but the current catch only handles
IO/Unauthorized/InvalidOperation; expand the exception filter on the catch
(currently "when (fileEx is IOException or UnauthorizedAccessException or
InvalidOperationException)") to also include ArgumentException,
NotSupportedException, and PathTooLongException so those errors are handled by
the static-file fallback (or alternatively validate/normalize the incoming
staticPath before constructing physicalPath); refer to the physicalPath,
resolvedPhysical, and the catch block surrounding Path.GetFullPath to locate the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 39ea5b09-3da0-452d-bc0f-c78e3aaf6749
📒 Files selected for processing (28)
web/Areas/CTS/Controllers/BundleCompetencyController.csweb/Areas/CTS/Controllers/BundleCompetencyGroupController.csweb/Areas/CTS/Controllers/BundleController.csweb/Areas/CTS/Controllers/CompetencyController.csweb/Areas/CTS/Controllers/CourseController.csweb/Areas/CTS/Controllers/DomainController.csweb/Areas/CTS/Controllers/EpaController.csweb/Areas/CTS/Controllers/LevelsController.csweb/Areas/CTS/Controllers/RoleController.csweb/Areas/ClinicalScheduler/Controllers/CliniciansController.csweb/Areas/ClinicalScheduler/Controllers/InstructorScheduleController.csweb/Areas/ClinicalScheduler/Controllers/PermissionsController.csweb/Areas/ClinicalScheduler/Controllers/RotationsController.csweb/Areas/Computing/Services/BiorenderStudentLookup.csweb/Areas/RAPS/Controllers/AdGroupsController.csweb/Areas/RAPS/Controllers/RoleMembersController.csweb/Areas/RAPS/Controllers/RolesController.csweb/Areas/RAPS/Services/OuGroupService.csweb/Areas/RAPS/Services/VMACSExport.csweb/Classes/ClaimsTransformer.csweb/Classes/SitemapMiddleware.csweb/Classes/UserHelper.csweb/Classes/Utilities/ActiveDirectoryService.csweb/Classes/Utilities/F5HttpRequest.csweb/Classes/Utilities/IamApi.csweb/Program.csweb/Services/EmailService.csweb/ViteProxyHelpers.cs
🛑 Comments failed to post (3)
web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs (1)
579-582:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the fallback contract in these permission helpers.
These methods are still written as fail-open / fail-closed boundaries (
return cliniciansandreturn false), but the narrowed filters now let any non-DB failure fromGetCurrentUser(),GetRequiredService<RAPSContext>(), orHasPermission(...)escape as a 500. That changesGetCliniciansandGetClinicianSchedulebehavior on permission-resolution errors. Move the broad catch to the action boundary or restore full fallback handling here.Also applies to: 657-660
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs` around lines 579 - 582, The permission helper methods currently only catch DbUpdateException/SqlException/InvalidOperationException so non-DB failures from GetCurrentUser(), GetRequiredService<RAPSContext>(), or HasPermission(...) surface as 500s and break the intended fail-open/fail-closed contract; update the helpers to catch all exceptions (catch Exception) and return the original fallback values (e.g., return clinicians or return false) so permission-resolution errors preserve the existing behavior, or alternatively move the broad try/catch to the controller action boundary (GetClinicians/GetClinicianSchedule) to preserve the contract — apply the same change to both occurrences around the blocks referencing GetCurrentUser(), GetRequiredService<RAPSContext>(), and HasPermission(...).web/Classes/Utilities/ActiveDirectoryService.cs (1)
222-225:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRoot cause: filtered
catch (Exception ...)is still used in service/utility code instead of typed catches.
web/Classes/Utilities/ActiveDirectoryService.cs,web/Classes/Utilities/F5HttpRequest.cs,web/Classes/Utilities/IamApi.cs, andweb/Services/EmailService.csall retain broadExceptioncatches with filters in non-boundary logic. Apply direct typed catch blocks in each location to comply with the project’s C# exception-handling standard.As per coding guidelines,
**/*.cs: “Prefer specific exception types ... Flag broadExceptioncatches inside service/business logic.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Classes/Utilities/ActiveDirectoryService.cs` around lines 222 - 225, Replace the filtered catch of a broad Exception with explicit typed catches in the service/business logic: remove constructs like "catch (Exception ex) when (ex is PrincipalException or System.DirectoryServices.DirectoryServicesCOMException)" and add separate catch blocks "catch (PrincipalException ex) { HttpHelper.Logger.Error(ex); }" and "catch (System.DirectoryServices.DirectoryServicesCOMException ex) { HttpHelper.Logger.Error(ex); }" (apply the same change in ActiveDirectoryService.cs, F5HttpRequest.cs, IamApi.cs, and EmailService.cs), ensure the appropriate namespaces are imported (e.g., System.DirectoryServices, System.DirectoryServices.AccountManagement) and do not leave a broad catch(Exception) in these non-boundary layers so other unexpected exceptions can propagate.Source: Coding guidelines
web/Program.cs (1)
473-475:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
LogSanitizerfor request-derived log values inweb/Program.csandweb/ViteProxyHelpers.cs.These sites log client-controlled paths/query values using
Uri.EscapeDataString,HtmlEncode, or newline stripping. That still bypasses the repository logging rule and keeps request data in logs. Sanitize withLogSanitizer, and avoid logging raw query strings / derived target URLs when the value originates from the request.As per coding guidelines, "Sanitize user input before logging via
LogSanitizer(SanitizeId(),SanitizeString(),SanitizeYear())."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Program.cs` around lines 473 - 475, The log is currently writing request-derived values (e.g., context.Request.Path/queries/derived target URLs) using Uri.EscapeDataString/HtmlEncode/newline-stripping which still allows client-controlled data into logs; update the call sites in Program.cs (viteLogger.LogDebug) and ViteProxyHelpers.cs to sanitize any request-originated strings with LogSanitizer (e.g., LogSanitizer.SanitizeString or SanitizeId as appropriate) before logging, and avoid logging raw query strings or full derived target URLs—either omit them or log a sanitized/placeholder value instead. Ensure you call the sanitizer on the specific variables passed into LogDebug/LogInformation and do not log context.Request.QueryString or generated target URL directly.Source: Coding guidelines
Match each catch filter to what the operation actually throws instead of the uniform DB-exception filter (which let HTTP/AWS/AD/JSON failures escape): - AWS Parameter Store (Program.cs): AmazonServiceException/AmazonClientException - HTTP send/retry (F5HttpRequest, VMACSExport): HttpRequestException + TaskCanceledException - JSON parsing (IamApi, VMACSExport.ParseResponse): JsonException/NotSupportedException - Active Directory (ActiveDirectoryService): PrincipalException/DirectoryServicesCOMException - response headers (ViteProxyHelpers): ArgumentException/InvalidOperationException - static-file fallback (ViteProxyHelpers): IOException/UnauthorizedAccessException - sitemap middleware boundary: broad catch with CA1031 + rationale (must always fall through to the pipeline) Drop OperationCanceledException from controller action filters so request cancellation propagates instead of being turned into a 500. Stop returning raw exception text to clients (EpaController, AdGroupsController, RolesController) and correct a wrong entity name in CompetencyController. Remove the now-orphaned EntityFrameworkCore/SqlClient usings.
GetGroups() result was discarded, leaving ldapGroups empty so BoxSyncEnabled was always false. Assign it, narrow the catch to the LDAP exception types the call actually throws, and pre-build set/map lookups instead of per-group FirstOrDefault scans.
Returning the unfiltered list on a permission-check exception exposed all clinicians to own-schedule-only users. Return an empty list instead.
…permit WaitAsync ran before validation but only spawned tasks (which release the permit) for valid emails, so each invalid email leaked a permit and 20+ invalid entries deadlocked the request.
7bd1864 to
94fa2f2
Compare
|
Addressed CodeRabbit's outside-diff comments (the ones that could not be posted inline): Fixed
Already addressed / stale
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
web/Areas/RAPS/Services/VMACSExport.cs:139
- These messages are returned to callers (via GetVMACSExportStatus), but they currently embed exception messages and full stack traces. That leaks internal implementation details to API/UI consumers and can include sensitive data. Log the exception server-side and return a fixed, non-detailed message in the user-facing messages list.
catch (Exception ex) when (ex is HttpRequestException or InvalidOperationException or TaskCanceledException)
{
HttpHelper.Logger.Log(LogLevel.Warn, ex);
RecordMessage(messages, "Error: " + ex.Message + " " + ex.StackTrace);
if (ex.InnerException != null)
web/Areas/RAPS/Controllers/RoleMembersController.cs:274
- This error path returns the unparsed final message content back to the caller ("Could not parse response from VMACS: " + finalMessage). In failure cases finalMessage may contain stack traces or raw upstream responses produced earlier in the export flow. Return a fixed message and avoid echoing raw content to the client.
catch (JsonException)
{
return new VmacsResponse
{
ErrorMessage = "Could not parse response from VMACS: " + finalMessage,
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException) | ||
| { | ||
| _logger.LogError(ex, "Error filtering clinicians by permissions. Returning unfiltered list."); | ||
| return clinicians; // Return unfiltered list on error to avoid breaking functionality | ||
| _logger.LogError(ex, "Error filtering clinicians by permissions. Returning empty list."); | ||
| return new List<ClinicianSummary>(); // Fail closed: don't expose unfiltered data when permission checks fail | ||
| } |
| vmacsResponse.Success = response.IsSuccessStatusCode; | ||
| } | ||
| catch (Exception) | ||
| catch (Exception ex) when (ex is JsonException or NotSupportedException or InvalidOperationException) |
| catch (Exception ex) when (ex is JsonException or NotSupportedException or InvalidOperationException) | ||
| { | ||
| r.ErrorMessage = "Invalid response: " + ex.Message + "."; | ||
| } |
Summary
Closes 38 of 46 remaining
cs/catch-of-all-exceptionsalerts. Same when-filter pattern as #193, applied across the rest of the codebase.Rebased onto latest
main(post #184). The earlierProgram.csVite-proxy narrowing was dropped in the rebase:main's SPA-shell refactor already scopes that catch toHttpRequestException/TaskCanceledException.Files changed (28)
Filter choices (matched to what each call site actually throws)
DbUpdateException/SqlException/InvalidOperationExceptionHttpRequestException/TaskCanceledExceptionJsonException/NotSupportedExceptionPrincipalException/DirectoryServicesCOMException; OuGroupServiceLdapException/DirectoryOperationExceptionAmazonServiceException/AmazonClientExceptionSocketExceptionplus config-argument failuresFormatException/ArgumentExceptionRaw exception messages are no longer returned to clients (EpaController, AdGroupsController); responses use fixed messages with details kept in server logs.
Drive-by fix surfaced by review:
OuGroupService.GetAllGroupsdiscarded theActiveDirectoryService.GetGroups()result, soBoxSyncEnabledwas always false. Now wired up (own commit).Intentional broad catches kept (the 8 alerts left open)
Program.cstop-level startup catch: must log fatal on any exception before rethrowing (#pragma warning disable CA1031)SitemapMiddleware: middleware boundary; any generation failure must fall through to_next(context)(restored to broad per review, with CA1031 pragma and rationale)EmailService.GetRequestContext: defensive log-context builderHangfireHealthCheck: must not crash the health response on any storage error (documented in-code)ScheduleEditServicepost-transaction catches kept broad in CodeQL 5: refactor(ClinicalScheduler): catch specific exceptions in services #193Test files (test/CTS/AssessmentControllerTest.cs helpers, test/ClinicalScheduler/EmailNotificationTest.cs diagnostic catches) are intentionally left as broad catches: they are test infrastructure, not production code.
Context
Sixth in the
CodeQL N:cleanup series (after #189, #190, #191, #192, #193).Test plan
npm run test:backend- all tests passing